-
-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Base refresh materialized view #800
base: master
Are you sure you want to change the base?
Conversation
Gerych1984
commented
Jan 22, 2024
Q | A |
---|---|
Is bugfix? | ❌ |
New feature? | ✔️ |
Breaks BC? | ❌ |
d38325b
to
020d468
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #800 +/- ##
=========================================
Coverage 99.64% 99.64%
- Complexity 1271 1277 +6
=========================================
Files 63 63
Lines 3106 3119 +13
=========================================
+ Hits 3095 3108 +13
Misses 11 11 ☔ View full report in Codecov by Sentry. |
020d468
to
348f586
Compare
348f586
to
4e86a29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think materialized views should be added in complex. With create
, alter
, drop
and refresh
methods. And take into account features of all supported DBMSs.
So, we should realize all these methods for all supported DBMSs which support materialized views.
That's what this PR is made for. Without it, i can't go any further |
* @param bool|null $withData | ||
* @return bool | ||
*/ | ||
public function refreshMaterializedView(string $viewName, ?bool $concurrently = null, ?bool $withData = null): bool; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function refreshMaterializedView(string $viewName, ?bool $concurrently = null, ?bool $withData = null): bool; | |
public function refreshMaterializedView(string $viewName, bool $concurrently = false, ?bool $withData = null): bool; |
Why the interface is different from DDLQueryBuilderInterface::refreshMaterializedView()
?
Looks like should be the same.
When execute $command->refreshMaterializedView($viewName);
expect this is not concurently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I see in MS SQL / Oracle / Clickhouse docs:
WITH [NO] DATA
is PostgreSQL only option and pretty much useless one.REFRESH
exists only on PG and Oracle. MS SQL and Clickhouse do automatic updates, so their implementations should always returntrue
.
There are a lot of variants in Oracle: 3 commands (REFRESH
,REFREASH_ALL_MVIEWS
,REFRESH_DEPENDENT
) and at least 6 options. Optionout_of_place
seems to be equivalent of CONCURRENTLY in PostgreSQL.
So, the most common part is refresh + option for non-blocking update, but it will be very hard to take full advantage of database capabilities like Oracle's refresh method.
$sql = $db->getQueryBuilder()->refreshMaterializedView($viewName, $concurrently, $withData); | ||
$actual = DbHelper::replaceQuotes($sql, $driver); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$sql = $db->getQueryBuilder()->refreshMaterializedView($viewName, $concurrently, $withData); | |
$actual = DbHelper::replaceQuotes($sql, $driver); | |
$actual = $db->getQueryBuilder()->refreshMaterializedView($viewName, $concurrently, $withData); |
Result from $db->getQueryBuilder()->refreshMaterializedView()
already quoted acording to the driver
@@ -178,7 +178,7 @@ public function testGetExpressionBuilderException(): void | |||
|
|||
$this->expectException(Exception::class); | |||
|
|||
$expression = new class () implements ExpressionInterface { | |||
$expression = new class() implements ExpressionInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this to the 2.0.0 milestone due to (almost) any change of interfaces breaks BC |
* | ||
* @return string The `REFRESH MATERIALIZED VIEW` SQL statement | ||
*/ | ||
public function refreshMaterializedView(string $viewName, bool $concurrently = false, ?bool $withData = null): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function refreshMaterializedView(string $viewName, bool $concurrently = false, ?bool $withData = null): string; | |
public function refreshMaterializedView(string $viewName, bool $concurrently = false): string; |
Not sure the argument $withData
is needed. If it is false
, it is equal to "truncate" the materialized view. Maybe better remove it and if the method is required add truncateMaterializedView()
public static function refreshMaterializedViewDataProvider(): array | ||
{ | ||
return [ | ||
[ | ||
'default_mt', | ||
null, | ||
null, | ||
], | ||
[ | ||
'concurrently_mt', | ||
true, | ||
null, | ||
], | ||
[ | ||
'concurrently_with_data_mt', | ||
true, | ||
true, | ||
], | ||
[ | ||
'concurrently_without_data_mt', | ||
true, | ||
false, | ||
], | ||
]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NotSupportedException
can be tested one time with one set of arguments.
public static function refreshMaterializedViewDataProvider(): array | ||
{ | ||
return [ | ||
[ | ||
'concurrently_mt', | ||
true, | ||
null, | ||
'REFRESH MATERIALIZED VIEW CONCURRENTLY [[concurrently_mt]]', | ||
], | ||
[ | ||
'concurrently_with_data_mt', | ||
true, | ||
true, | ||
'REFRESH MATERIALIZED VIEW CONCURRENTLY [[concurrently_with_data_mt]] WITH DATA', | ||
], | ||
[ | ||
'concurrently_without_data_mt', | ||
true, | ||
false, | ||
'REFRESH MATERIALIZED VIEW CONCURRENTLY [[concurrently_without_data_mt]] WITH NO DATA', | ||
], | ||
[ | ||
'not_concurrently_mt', | ||
false, | ||
null, | ||
'REFRESH MATERIALIZED VIEW [[not_concurrently_mt]]', | ||
], | ||
[ | ||
'not_concurrently_with_data_mt', | ||
false, | ||
true, | ||
'REFRESH MATERIALIZED VIEW [[not_concurrently_with_data_mt]] WITH DATA', | ||
], | ||
[ | ||
'not_concurrently_without_data_mt', | ||
false, | ||
false, | ||
'REFRESH MATERIALIZED VIEW [[not_concurrently_without_data_mt]] WITH NO DATA', | ||
], | ||
]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to move it to QueryBuilderProvider
* @param bool|null $withData | ||
* @return void | ||
*/ | ||
public function testRefreshMaterializedView(string $viewName, bool $concurrently, ?bool $withData, string $expected): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks need to move this test to AbstractQueryBuilderTest
From what I see in MS SQL / Oracle / Clickhouse / PostgreSQL docs:
|